Drop Python 3.9, use 3.10 for dist, bump Ledger - #819
Conversation
8489c74 to
57f6690
Compare
57f6690 to
a608ea3
Compare
|
Rebased after #818 landed, ready for review. I lightly tested the dist builds on Ubuntu (x86_64, including qt) and macOS (only command line). I ran the GUI in Wine, but that doesn't have USB access so not very informative. I also ran it on a native Windows machine. Windows Defender wasn't happy about it, but I didn't do any code signing. There it detected a connected device, so that's good. I wonder if it makes sense for the deterministic binary releases to jump straight ahead to a more modern Python? Also, would it make sense to do one final HWI release with Python 3.9 before merging this? |
|
Added commit to re-enable the Ledger tests. |
a4e3a84 to
ae9e49d
Compare
|
Rebased just in case after recent merges. |
|
Does the dist build work on in an arm docker container, as described in the release process? |
ae9e49d to
f473a2e
Compare
I built inside an arm64 docker container on the x86 linux machine. I also made it do an Rebased again, this time using The first time I ran that command, it moved I also built and tested the x86 macOS build on a native machine. I was able to run the QT application and it connected to a test deviced and fetch the descriptors. Checksums: |
|
The spurious Ledger failure appears to be the result of the test suite occasionally hitting the Cancel / Reject button. Where Added a commit to drop that rule, since it's unused anyway. |
3ac16e0 to
5197ac2
Compare
|
Rebased just in case.
I briefly tested the x86 linux gui. |
|
Added a commit here to fix Trezor CI. Unrelated so can be its own PR if it works. |
c85d39c to
a8bd342
Compare
|
The one bitbox01 and one ledger failures seem spurious. But the jade test failures were real, so I'm pushing a new fix shortly (basically #832). |
d2093d9 to
d21b545
Compare
|
Actually #832 did not do the trick. This time I ran the simulator locally to reproduce the issue. It was the PSBTv2 change on Bitcoin Core's master (again, this change bit me multiple times). I dropped the cbor commits, because they're not needed here, but based on local testing they should not be a problem. I opened a fresh PR for just the CI fixes: #836 |
d21b545 to
712823a
Compare
|
The one failing ledger just is spurious. @andreasgriffin much of this was written with the help of Claude and Codex, so I wouldn't expect it to find anything. Unless you give it a more specific prompt based on your own review, of something that might be incorrect. In general for this Python bump it would be great if someone can test the binaries on every supported platform. I did that before the rebase, so I don't expect anything new to have broken, but still. |
712823a to
887843c
Compare
|
Fable discovered that the one flaky failure here was due to a base58 address ending in It's a pre-existing bug, but I kept the fix be87c6e here, because it only impacts tests that were disabled before this PR. |
|
@achow101 Codex says it's your fault :-)
|
831193d to
76bd097
Compare
Well, since I do the releases, if it doesn't work on my machine, it ain't gonna be merged. |
|
@achow101 did the suggestion work? I can't reproduce the issue, so will need more details. |
|
The current error is |
|
Apparently I have to disable ASLR... |
| git clone --branch 2.4.6 --depth 1 https://github.com/LedgerHQ/app-bitcoin.git | ||
| cd app-bitcoin | ||
| make DEBUG=1 BOLOS_SDK=$NANOX_SDK | ||
| # Work around register-wallet policy parsing/display bugs fixed after v2.4.6. |
There was a problem hiding this comment.
In 50da3f1 "Bump Speculos and Ledger Bitcoin app"
Can we instead pin a commit that has these included instead of cherry picking them? We're already building from source anyways.
There was a problem hiding this comment.
I bumped to 2.5.0, which has been released since and contains these fixes.
| # Work around -Werror build failures in Speculos' bundled deps. | ||
| # GCC < 15 errors out on unknown "-Wno-error=..." options, so only add the | ||
| # unterminated-string-initialization suppression when the compiler supports it. | ||
| CFLAGS="-O -fno-builtin -fPIC -Wall -Wextra -Werror -Wno-error=maybe-uninitialized -Wno-error=array-parameter -Wno-error=array-bounds -Wno-error=discarded-qualifiers" |
There was a problem hiding this comment.
In 50da3f1 "Bump Speculos and Ledger Bitcoin app"
Where did these flags come from? I don't see them in the speculos codebase at all.
There was a problem hiding this comment.
Dropped. Not sure if this was useful in earlier versions (it's been there since at least January), but it's definitely not needed now.
There was a problem hiding this comment.
It looks like my Ubuntu test machine needs it, but CI doesn't, so I will leave it out.
76bd097 to
8ae811e
Compare
Most device simulators accept arbitrary keypool paths, so keep testing that behavior by default. Some devices enforce their own derivation path policies. Let those emulators opt out of the arbitrary-path portion while still running the remaining keypool checks.
8ae811e to
ac68456
Compare
|
Rebased after #838 and addressed inline feedback. |
ac68456 to
0bf6d7a
Compare
|
The Ledger app bump from 2.4.6 to 2.5.0 required another automation update, due to LedgerHQ/app-bitcoin@d452557, LedgerHQ/app-bitcoin@26f631c and LedgerHQ/app-bitcoin@f883a91. |
Ledger app 2.5.0 can take over 60 seconds to sign the large transaction fixture. Allow stdin commands up to 120 seconds.
It's unused and occasionally trips up a test.
Limit the warning rule to the centered title so it does not also click the repeated header on the details screen. This prevents the queued extra click from selecting Back to safety before Continue anyway is confirmed.
0bf6d7a to
d27d196
Compare
|
I added cd8a431 to double the timeout for the ledger stdin jobs. IIUC the Part of the problem is that logging is so verbose, that it slows the test down. @bigspider can you demote the packet/byte chatter from DEBUG to TRACE? The (more interesting) screen read and button press information is also at DEBUG, so it's currently non-trivial to disentangle. And log files are ~400MB. |
I think dropping |
|
@achow101 it does, but we also lose the (very useful) |
This PR first switches the deterministic builds to Python 3.10. It then drops the end-of-life 3.9.
Then it bumps Speculos and Ledger and enables the tests that were previously disabled.
There's also a few minor cleanup commits, plus changes to make the Docker containers Podman friendly so I can easily run them locally.